Controller unhandled errors should be passed as BadStatus to interceptors#184
Open
ximus wants to merge 1 commit intobigcommerce:mainfrom
Open
Controller unhandled errors should be passed as BadStatus to interceptors#184ximus wants to merge 1 commit intobigcommerce:mainfrom
ximus wants to merge 1 commit intobigcommerce:mainfrom
Conversation
f1ba5a6 to
436dfbf
Compare
Contributor
Author
|
wondering if interceptors like |
436dfbf to
68e74d6
Compare
68e74d6 to
3ee49c9
Compare
3ee49c9 to
7294072
Compare
Contributor
Author
|
@splittingred I still think this PR makes sense. I think it's pretty straight forward, any chance we could move it forward? |
…tors
While using `Gruf::Interceptors::Instrumentation::RequestLogging::Interceptor`, I noticed that in the event of of unhandled errors raised in my controller (errors that are not of type `GRPC::BadStatus` or raised via `fail!`), I was not getting any request logging in my log.
In a successful controller call I would get something like:
```
[GRPC::Ok] (list_units) [5.24ms] [GRPC::Ok] (rpc.units.list_units) Parameters: {:is_active=>false, :check_can_delete=>false}
```
If an unhandled error was raised I would get nothing.
My assumption is that I should still get logging in that case. Request info is useful to have, including when an unhandled error occurs.
Investingating why I wasn't getting any, I observed that in `Gruf::Interceptors::Instrumentation::RequestLogging::Interceptor`, the interceptor yields to the controller via this line:
```ruby
result = Gruf::Interceptors::Timer.time(&block)
```
`Gruf::Interceptors::Timer.time` knows to `rescue` and handle errors, but only of type `GRPC::BadStatus`. I realized that unhandled errors would not be caught by it, hence the error logging interceptor would get exited right there and never log.
I noticed that the base controller code turns unhandled errors in the `GRPC::BadStatus` errors. But it only does so after all the interceptors have run (or been tripped as above). I thought why not just do this before the interceptors. Seeing how the logging interceptor is designed, maybe it was even meant to be that way. So this is what I propose here.
It works well for me so far.
API changes:
* Now interceptors will get a `GRPC::BadStatus` error in the case of unhandled errors, instead of the original unhandled error. To reach the original error, `GRPC::BadStatus#cause` can be used.
7294072 to
6c548de
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

While using
Gruf::Interceptors::Instrumentation::RequestLogging::Interceptor, I noticed that when unhandled errors raised in my controller (errors that are not of typeGRPC::BadStatusor raised viafail!), I was not getting any request logging in my log.In a successful controller call I would get something like:
If an unhandled error was raised I would get nothing.
My assumption is that I should still get logging in that case. Request info is useful to have, including when an unhandled error occurs.
Investingating why I wasn't getting any, I observed that in
Gruf::Interceptors::Instrumentation::RequestLogging::Interceptor, the interceptor yields to the controller via this line:Gruf::Interceptors::Timer.timeknows torescueand handle errors, but only of typeGRPC::BadStatus. I realized that unhandled errors would not be caught by it, hence the error logging interceptor would get exited right there and never log.I noticed that the base controller code turns unhandled errors in the
GRPC::BadStatuserrors. But it only does so after all the interceptors have run (or been tripped as above). I thought why not just do this before the interceptors. Seeing how the logging interceptor is designed, maybe it was even meant to be that way. So this is what I propose here.It works well for me so far.
API changes:
GRPC::BadStatuserror in the case of unhandled errors, instead of the original unhandled error. To reach the original error,GRPC::BadStatus#causecan be used.